Conversation
WalkthroughReorganizes many HTML settings pages into new visual sections ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@DedeHai I like the new look :-) |
|
correct. |
|
I updated all config pages and made some other minor modifications, mainly:
Total additional flash use: ~150bytes |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/data/settings_sec.htm`:
- Around line 62-70: The HTML has invalid nesting because the inline element
<span id="OTA"> opens before a </div> and closes after several block elements;
move or refactor this so the element with id "OTA" does not span across
block-level boundaries — either wrap the relevant block content inside a single
container (e.g., give the <div class="sec"> or a new <div> the id "OTA" instead
of using a span) or remove the span and toggle visibility via the existing <div
class="sec"> using the same id; ensure related JS (function U(), and inputs
named "AO" and "SU") references are updated to target the new container.
after LED settings now come color & white settings, then additional hardware, then general settings. Also updated some wordings to avoid FAQ.
|
@netmindz what do you think of this? it's ready to merge, there are zero functional changes, only cosmetics. |
|
I see it has conflicts due to recent addition to user_fx, ready to merge after a quick fixup. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/data/settings_sync.htm (1)
127-128:⚠️ Potential issue | 🟡 Minor"Send Alexa notifications" checkbox visible but non-functional when Alexa is compiled out.
The
SAcheckbox (line 127) is in the always-visible "Send" section, outside the Alexa-specific<div id="Alexa">that gets hidden whenWLED_DISABLE_ALEXAis defined. However, inxml.cpp, theSAvalue is only populated inside#ifndef WLED_DISABLE_ALEXA(line 497), and when Alexa is disabled,toggle('Alexa')is called (line 500) which hides only the Alexa divs—not the "Send" section. This leaves the SA checkbox visible but unchecked and non-functional.Move the SA checkbox into the Alexa-specific
<div id="Alexa">(insidesettings_sync.htm), or wrap it in a separate conditionally-hidden element.The
SHcheckbox (line 128) is correctly populated unconditionally inxml.cpp.









I am proposing "sections" instead of a "continuous list"

like this:
had to increase the contrast slightly to make it work well, i.e. dim the background down.
let me know what you think and I will update all config pages to this pattern.
the flash cost is minimal, changes to LED settings page is a handful of bytes (64 if you need a number ;) )
Summary by CodeRabbit
Style
Refactor
New Features
Other